HADOOP-19342. SaslRpcServer.AuthMethod print INFO messages in client side#7173
Closed
szetszwo wants to merge 4 commits intoapache:trunkfrom
Closed
HADOOP-19342. SaslRpcServer.AuthMethod print INFO messages in client side#7173szetszwo wants to merge 4 commits intoapache:trunkfrom
szetszwo wants to merge 4 commits intoapache:trunkfrom
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
stoty
reviewed
Nov 19, 2024
| this.code = code; | ||
| this.mechanismName = mechanismName; | ||
| LOG.info("{} {}: code={}, mechanism=\"{}\"", | ||
| LOG.debug("{} {}: code={}, mechanism=\"{}\"", |
Contributor
There was a problem hiding this comment.
AFAICT this ultimately gets called at roughly the same point during client initialization as previously, it just doesn't show up because the debug level has been reduced to DEBUG from INFO.
Contributor
There was a problem hiding this comment.
So apart from the log level the only difference is that this will pick up changes in the config
if a new SaslRPCServer is created at a later time ?
Contributor
There was a problem hiding this comment.
if you aren't using SASL stuff saslMechanismFactory::getMechanism doesn't get invoked.
Contributor
Author
There was a problem hiding this comment.
... this will pick up changes in the config if a new SaslRPCServer is created at a later time ?
It will load the config only when SaslMechanismFactory.getMechanism is called. So, it won't be loaded with UGI.
|
💔 -1 overall
This message was automatically generated. |
steveloughran
requested changes
Nov 19, 2024
Contributor
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
production code: +1 from me;
test changes requested..overwriting core-site.xml is too dangerous for my liking, especially if hadoop-common test runner ever goes to parallel execution across JVMs
| */ | ||
| package org.apache.hadoop.security; | ||
|
|
||
| import org.apache.hadoop.conf.Configuration; |
Contributor
There was a problem hiding this comment.
nit: split out org.apache and put in a separate block below
| try (OutputStream out = Files.newOutputStream(Paths.get(coreSite), StandardOpenOption.CREATE)) { | ||
| conf.writeXml(out); | ||
| } | ||
| Configuration.addDefaultResource(coreSite); |
Contributor
There was a problem hiding this comment.
this is going to contaminate all tests which follow in the same JVM, and make debugging conf problems a lot harder.
what about
- create a new xml conf resource adjacent to core-site.xml
- give it the name of this test
- modify this method so if mechanism == null, the conf option is not set.
- after the test, you could set to being empty XML. (then load a configuration to make sure it still loads)
Configuration conf = new Configuration(false)
if (mechanism != null) conf.set(HADOOP_SECURITY_SASL_MECHANISM_KEY, mechanism);
String newResource = /* work this out */
conf.write(new resource)
Configuration.addDefaultResource(newResource);
and in and @after method, use initConf(null) to reset it.
Avoids overwriting core-site.xml
Contributor
Author
There was a problem hiding this comment.
You are right that it contaminates all tests.
Tried multiple things but not working very well. One problem is that Configuration has addDefaultResource but not removeDefaultResource. Let skip the test.
| import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.HADOOP_SECURITY_SASL_MECHANISM_KEY; | ||
|
|
||
| /** Test {@link SaslRpcServer.AuthMethod}. */ | ||
| public class TestAuthMethod { |
Contributor
There was a problem hiding this comment.
extends AbstractHadoopTestBase for timeouts, thread names etc
| this.code = code; | ||
| this.mechanismName = mechanismName; | ||
| LOG.info("{} {}: code={}, mechanism=\"{}\"", | ||
| LOG.debug("{} {}: code={}, mechanism=\"{}\"", |
Contributor
There was a problem hiding this comment.
if you aren't using SASL stuff saslMechanismFactory::getMechanism doesn't get invoked.
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
The test failure is related. Looking at it. |
Contributor
Author
|
Actually, the test failure is not related. Seems the Yetus build has something messed up. Will try a new PR #7174 |
Contributor
Author
|
Using a new pr #7174 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of PR
HADOOP-19342
After HADOOP-19306, the following INFO messages are printed in client side. Thanks As Steve Loughran for reporting it in this comment.
How was this patch tested?
Manual test.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?